Output an accurate error message when unimplemented math functions are called (globally) #1501
Conversation
|
(Taking review, since I'm mentoring @sjinno on this work.) |
src/front/glsl/parser.rs
Outdated
| @@ -368,7 +368,7 @@ impl Parser { | |||
|
|
|||
| pub struct DeclarationContext<'ctx> { | |||
| qualifiers: Vec<(TypeQualifier, Span)>, | |||
| external: bool, | |||
| external: bool, // Indicates global declaration | |||
There was a problem hiding this comment.
Even though DeclarationContext is private, and will not appear in documentation unless someone builds with --document-private-items, I think this should still be a /// doc comment, placed above the field, because that's what people are expecting to see.
| init.and_then(|(root, meta)| parser.solve_constant(ctx.ctx, root, meta).ok()); | ||
| let maybe_constant = if let Some((root, meta)) = init { | ||
| let result = parser.solve_constant(ctx.ctx, root, meta); | ||
| match (result, ctx.external) { |
There was a problem hiding this comment.
This looks correct, but I think it might be clearer as a plain match result, with a guard expression on the Err case:
Err(err) if ctx.external => return Err(err),
It also deserves a little comment, something like:
Initializers for external declarations must be constants, so if solve_constant failed, that's an error the user should see. But if it's an initializer for a local variable, then failing to produce a constant just means we emit the expression as code; that is not an error.
There was a problem hiding this comment.
Thank you very much for the review! I'd like to use what you wrote as a comment. :)
There was a problem hiding this comment.
Currently maybe_constant assigns the returned value of sove_constant for both externally and internally declared math functions if implemented.
So, should it also be
Ok(res) if ctx.external => Some(res)
so that internally declared math expressions are left as is?
sjinno
force-pushed
the
1498-print-proper-error-message
branch
2 times, most recently
from
648ab6c
to
173ab74
Compare
sjinno
force-pushed
the
1498-print-proper-error-message
branch
from
9ac3f3c
to
cd5fbdb
Compare
There was a problem hiding this comment.
Implementation wise it looks good to me, I'll let @jimblandy handle the rest since he is mentoring you
|
@JCapucho Thank you for your help! |
Co-authored-by: JCapucho <jcapucho7@gmail.com>
JCapucho
force-pushed
the
1498-print-proper-error-message
branch
from
a4ac636
to
55e8379
Compare
|
Since the author hasn't touched this PR in a while, I've gone ahead and rebased it and cleaned it up a little, the code is essentially the same, only that now the constants are also folded for non const declarations (as was previously done), I reworded the comments a bit as was requested by jimb in his review and changed the error message to be a bit more helpful. I've also cleaned the git history since it picked up many unneeded commits. @jimblandy it still needs an approval from you, so if you could do a review it would be very helpful. |
An accurate error message wasn't output when unimplemented math functions were called.
In this case (#1498),
would say
error: const values must have an initializerwhen it should actually be sayingNote: This change only applies to unimplemented math functions that are called globally (or should I say externally?).
Feedback on wording/how code should be written is very much appreciated! :)